security(star-rating): authorize the /o?method=star ratings read - #7955
Open
ar2rsawseen wants to merge 6 commits into
Open
security(star-rating): authorize the /o?method=star ratings read#7955ar2rsawseen wants to merge 6 commits into
ar2rsawseen wants to merge 6 commits into
Conversation
The star-rating dashboard read performed no authorization. Authentication on /o is per method: every core method calls validateUserForDataReadAPI itself, and the default branch hands the validators to plugins as helpers without calling them, so a plugin that claims a request is responsible for authorizing it. This branch claimed the request, returned true, and never called a validator, so the endpoint answered callers with no account, token or session, for any app_id they supplied. What it disclosed is the set of platform and application-version combinations that have received ratings for that application. Rating comments and the detailed feedback in /o/feedback/data were not affected; those reads are authorized. Wrap the branch in validateRead(params, FEATURE_NAME, ...), which is the same check the sibling reads in this file already apply (/o/feedback/data and /o/feedback/widgets). Authorization runs before the period parameter is validated, so an unauthorized caller cannot probe the endpoint through its error responses. app_id needs no extra guard here: the core /o case rejects a request without one, and this branch only concatenates it into a collection name, so there is no ObjectID conversion that could throw for a global admin whose validateRead call does not require app_id. The only caller is the dashboard Ratings page (plugins/star-rating/frontend/public/javascripts/countly.models.js, starRatingPlugin.requestPlatformVersion), which sends the session credential and an app_id the member has access to, so it is unaffected. Reported through the security bug bounty programme (received 2026-08-18). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ar2rsawseen
commented
Aug 24, 2026
| } | ||
| doc.meta.platform_version_rate.forEach(function(item) { | ||
| var data = item.split('**'); | ||
| if (result[data[0]] === undefined) { |
Member
Author
There was a problem hiding this comment.
[P2] Use a null-prototype map for stored platform names
data[0] comes from the public star-rating event's platform_version_rate segmentation and is used as an object key. Values such as __proto__, constructor, or toString resolve inherited members on {}, so this condition does not initialize an array and the following .indexOf() throws, making the authorized ratings read fail on attacker-seeded data. Build result with Object.create(null) or use an own-property check and explicitly initialize each key. The equivalent accumulator in the alternate/granular branch needs the same treatment.
Member
Author
There was a problem hiding this comment.
Confirmed, and it is a denial rather than a corruption — fixed in 10dcb09.
Ran the loop rather than reasoning about it. Against a plain object, each of these throws:
TypeError: result[data[0]].indexOf is not a function
for __proto__ (reads back as Object.prototype), constructor (the Object function), and toString / valueOf / hasOwnProperty (functions). None is undefined, so the "not seen yet" branch never runs and the array is never created. One planted row therefore fails the read for every authorized caller until it ages out — and the name comes off the public star-rating event's platform_version_rate segmentation, so anyone who can write to the app chooses it.
Took the Object.create(null) option:
var result = Object.create(null);Nothing downstream changes — the keys are still ordinary strings, and JSON.stringify serialises a null-prototype object identically, which is what returnOutput does with it. There is a test pinning that.
And the granular branch too, as you asked: the platform copy has a second accumulator over data2.data[z]._id.split('**') and it gets the same treatment. The server copies only have the one.
On the tests: they lift both the accumulator's declaration and the loop out of the real source, rather than the loop alone. That matters here — a test that built its own result would pass whichever object the shipping code used. Four of the five fail against the previous code, with the TypeError above.
# Conflicts: # CHANGELOG.md
result[data[0]] === undefined is not a "have I seen this platform" test on a plain object.
The platform name comes off the public star-rating event, in the platform_version_rate
segmentation, so anyone who can write to the app chooses it, and "__proto__",
"constructor", "toString", "valueOf" and "hasOwnProperty" all read back as inherited
members rather than as undefined. The array is then never created and the next line throws
TypeError: result[data[0]].indexOf is not a function
so one planted row denies the whole read for every authorized caller until it ages out.
Verified by running the loop, not by reading it.
Built with Object.create(null) instead. Nothing else changes: the keys are still ordinary
strings, and JSON.stringify serialises a null prototype object identically, which is what
returnOutput does with it. The platform has a second accumulator in the granular branch
and it gets the same treatment.
Tests lift both the accumulator's declaration and the loop out of the real source, so the
choice of object is what is under test rather than one the test made for itself. Four of
the five fail against the previous code, with the TypeError above.
ar2rsawseen
added a commit
that referenced
this pull request
Aug 25, 2026
The four two-factor-auth cases I added were the only new failures in test-api-core on
this branch. They allowed 403 or 404, on the assumption that a build without the plugin
answers 404. It does not: requestProcessor answers 400 "Invalid path", so the cases
failed everywhere the plugin is not enabled - which is the CI test build.
Widening the allowed set to include 400 would have been the wrong repair. A case that
accepts 400, 403 and 404 passes whether or not the guard exists, which is worse than no
case at all. They now probe once for the route and skip with a reason when it is absent,
and assert exactly 403 when it is there.
Skipping leaves the guard unproven in this build, so the coverage that does not depend on
the plugin being installed is added beside it: a unit suite that reads the plugin source
and requires refuseScopedCredential at each method that mutates the factor, before the
write rather than after it, and requires the global-admin methods to keep using
validateUserForGlobalAdmin. Removing the guard from one arm fails it, checked by doing
exactly that. On 24.05 the generate-qr-code case reports pending, because that branch has
no such method.
For the record on the rest of this branch's test-api-core run: the other three failures
("user permission when app is deleted", "correct admins and users", "should return one
user") are not from this change. The same three fail on #7930, which carries none of it,
and test-api-core passed in the same hour on #7923, #7935, #7941, #7955, #7970, #7894 and
#7871. That is the flaky trio, not a regression.
ar2rsawseen
added a commit
that referenced
this pull request
Aug 25, 2026
The four two-factor-auth cases I added were the only new failures in test-api-core on
this branch. They allowed 403 or 404, on the assumption that a build without the plugin
answers 404. It does not: requestProcessor answers 400 "Invalid path", so the cases
failed everywhere the plugin is not enabled - which is the CI test build.
Widening the allowed set to include 400 would have been the wrong repair. A case that
accepts 400, 403 and 404 passes whether or not the guard exists, which is worse than no
case at all. They now probe once for the route and skip with a reason when it is absent,
and assert exactly 403 when it is there.
Skipping leaves the guard unproven in this build, so the coverage that does not depend on
the plugin being installed is added beside it: a unit suite that reads the plugin source
and requires refuseScopedCredential at each method that mutates the factor, before the
write rather than after it, and requires the global-admin methods to keep using
validateUserForGlobalAdmin. Removing the guard from one arm fails it, checked by doing
exactly that. On 24.05 the generate-qr-code case reports pending, because that branch has
no such method.
For the record on the rest of this branch's test-api-core run: the other three failures
("user permission when app is deleted", "correct admins and users", "should return one
user") are not from this change. The same three fail on #7930, which carries none of it,
and test-api-core passed in the same hour on #7923, #7935, #7941, #7955, #7970, #7894 and
#7871. That is the flaky trio, not a regression.
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
The changelog is generated from PR and commit titles later, so an entry written by hand here is duplicated work at best. It is also the single worst file in this wave for conflicts: every merge to the base appends a line, which re-conflicts every open branch that also appends one. Eight of the sixteen conflicts across these security PRs today were this file and nothing else, and two of them came back within the hour. Only the lines this branch added are removed - the file is otherwise the base's, and the change here was purely additive, so nothing else moves.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/o?method=starreturns the platform and application-version combinations that have received star ratings. It performed no authorization, so any caller who supplies anapp_idreceived that application's data with noapi_key, noauth_tokenand no session.Measured against the running API with no credentials at all:
Why it was reachable
Authentication on
/ois per method, not global. Every core method authorizes itself (case 'durations'→validateUserForDataReadAPI(...),case 'events'→validateUserForDataReadAPI(...)), and thedefault:branch hands the validators to plugins as helpers:default: if (!plugins.dispatch(apiPath, { params: params, validateUserForDataReadAPI: validateUserForDataReadAPI, // passed, never called ... })) { common.returnMessage(params, 400, 'Invalid path, ...'); }dispatchreturns truthy when a plugin claims the request, and the only fallback is400 Invalid pathwhen nothing does. So a plugin registering on/omust authorize the request itself. Every other plugin does (views/api/api.js:879→validateReadat :886;times-of-day:177→ :185), and this plugin does it in its own siblings (/o/feedback/data:1258→ :1302,/o/feedback/widgets:1420→ :1422). Themethod === 'star'branch claimed the request, returnedtrue, and never called a validator.This was an omission rather than a design choice: the only caller is the dashboard Ratings page (
starRatingPlugin.requestPlatformVersioninplugins/star-rating/frontend/public/javascripts/countly.models.js), which sends the session credential and the active app. The plugin's deliberately public endpoints are a separate, clearly named family (/o/sdk,/feedback/widgets,/i/feedback/input).Impact
Cross-application disclosure of a specific customer's platform and application-version inventory, to an unauthenticated caller who knows the
app_id. Rating comments and detailed feedback live in/o/feedback/data, which is authorized and unaffected. Medium under SECURITY.md.Fix
Wrap the branch in
validateRead(params, FEATURE_NAME, …), the same check the sibling reads already apply. Authorization runs before theperiodparameter is validated, so an unauthorized caller cannot probe the endpoint through its error responses.app_idneeds no extra guard: the core/ocase rejects a request without one, and this branch only concatenates it into a collection name, so there is noObjectID()conversion that could throw for a global admin (whosevalidateReaddoes not requireapp_id). The diff is mostly re-indentation from introducing the callback.Scope of the class
Every plugin
/ohandler in countly-server, countly-platform and countly-enterprise-plugins was swept for a missing validator, and the surviving read candidates were probed unauthenticated.method=staris the only unauthenticated tenant-data read. The other credential-less responders return hardcoded lookup tables that ignoreapp_id(/o/langmap,/o/sources), are widget configuration for end-user SDKs (/o/feedback/widget), or are the intentional hooks API-endpoint trigger (/o/hooks)./o/surveys/*,/o/calculated_metrics/*and/o/journey-engine/foldersall authorize correctly.Reported through the security bug bounty program (received 2026-08-18).
🤖 Generated with Claude Code